Yes, but who knows what might end up happening in the engine 🤷 Might as well exercise as many situations as we can. Without looking at the V8 code, it seems unlikely but plausible that there would be some different code path taken on traps or exceptions, or when doing a JSPI suspension or something, depending on the return type even when the function never does a normal return.

Yeah, I guess if something unrelated changes the output we would need to entirely regenerate the test.

I still wish there were a more declarative way of doing this kind of property testing on our fuzzers, though. Can we at least refactor this into a generic utility for running the fuzzer iteratively and checking that the output satisfies various user-provided predicates with given probabilities? Laying this into reasonable abstractions would help make it more palatable.

Might as well check these conditions before burning a bit to generate a new exactness above.

Bump.

Ok, saved that bit.

A random subtype of the old result type, yes. The maybeRefine call below should not need to change.

Bump.

IIUC, this looks for a single iteration where all of these things are true. But that's stricter than we want. Can we make each of these conditions a bool on the class, set them to true whenever we see them, and pass the test once all have been seen?

This test's target properties all increase monotonically anyhow: as more things are seen, we only ever add them, making passing more likely.

Thanks, I see.

Why not keep passing the new type in as a Type? The only interesting thing we do with newLUB below is check newLUB.noted(), but shouldn't we be able to just check whether the new type (i.e. the LUB) is Type::unreachable?

Either could work, but noted() is a more explicit way to check whether we saw any type there, I think?

I don't think it's worth passing all this extra external state into the lambda, though.

LUBFinder is just a type + logic to track whether we noted, so it seems suitable?

Also note that the LUB tracks all the indexes at once (they are all unreachable or not), hence we pass in the index after it. This avoids each caller needing to check the unreachable case and indexing only if not.

It just seems like a simpler abstraction to me if maybeRefine just takes the new and old types, but I don't want to block landing on this.

I refactored in the last commit to make that method just use types, but there is an external maybeRefineIndex that takes the entire types and parses unreachability (that avoids the duplicated code that I want to prevent here).

I think this could all be replaced with the following:

auto lub = newLUB.noted() ? newLUB.getLUB()[index] : Type(Type::unreachable);
return maybeRefine(oldTypes[index], lub);

if we also added this to maybeRefine:

if (new_ == Type::unreachable) {
  new_ = Type(oldHeapType.getBottom(), NonNullable);
}

But I see that the noted and non-noted cases requiring different indexing is the source of the extra work. We could fix that by allowing arbitrary indexing into Type::unreachable (and having it return Type::unreachable at every index), but it's probably not worth making that change to simplify just this code.

Whatever way of structuring this you think is best SGTM.

Ah yes, handling unreachable in maybeRefine is a little simpler, thanks. Done.